Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[12.x] Fix proration behaviour being forced when syncing tax rates #1028

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

jackwh
Copy link
Contributor

@jackwh jackwh commented Nov 12, 2020

I recently updated my Cashier implementation to migrate customers from the tax_percent property to Stripe's Tax Rates.

I tried using the syncTaxRates() function to update existing customer subscriptions, but found this would always prorate by default. The screenshot below from Stripe's dashboard shows what was happening to existing customers with existing subscriptions when $user->subscription('default')->syncTaxRates(); was being called. A pair of "Remaining time" invoice items were being added unexpectedly:

Screenshot 2020-11-12 at 16 24 08

This change now allows users to call $user->subscription('default')->noProrate()->syncTaxRates();, should they wish, to prevent prorating when syncing tax rates.

This shouldn't break existing code, as the default behaviour is to still prorate. This just allows proration behaviour to be overridden when needed.

This allows for synchronising tax rates for subscriptions and subscription items without automatically prorating them.
@driesvints
Copy link
Member

Forgive me but I'm a bit confused here. Isn't the problem that the proration behavior wasn't set properly when the subscription was created? Afaik syncing tax rates with existing subscriptions doesn't alter's the subscription's proration behavior.

@driesvints driesvints changed the title Fix proration behaviour being forced when syncing tax rates [12.x] Fix proration behaviour being forced when syncing tax rates Nov 12, 2020
@jackwh
Copy link
Contributor Author

jackwh commented Nov 12, 2020

@driesvints Hopefully I'm understanding your comment correctly... From what I can see, proration behaviour doesn't "persist". Whether a subscription was created with prorating or not, any future updates to it will always prorate, unless explicitly disabled with each subsequent API request.

I might be wrong, but here's why I think this is the case...

  1. The Proration docs says prorating is the default behaviour. To override this, set proration_behavior in the API call.
  2. The Subscription object doesn't contain any fields suggesting proration behaviour is stored against the subscription.
  3. All my customer's subscriptions were created with noProrate() — but any future updates still prorate by default.

Basically, this PR just lets you move subscriptions from one Tax Rate to another, without creating pairs of unwanted proration adjustments.

I imagine you would usually want to prorate tax changes (hence it's still the default), but there's two examples I can see where you would not want proration:

  1. If a tax rate changes, but the tax is only incurred at the time the customer pays.
  2. In migrations, testing environments, or as part of refactoring (as I found out) — where you might want to apply new Tax Rates without adjusting invoices.

If I'm misunderstanding, let me know! But hopefully that explains it a bit better 😄

@driesvints
Copy link
Member

@jackwh I've send a message to my Stripe contacts about this. In the API docs there's no mention of proration being altered when a subscription's default tax rates are updated: https://stripe.com/docs/api/subscriptions/update#update_subscription-proration_behavior

Determines how to handle prorations when the billing cycle changes (e.g., when switching plans, resetting billing_cycle_anchor=now, or starting a trial), or if an item’s quantity changes.

@cjavilla-stripe
Copy link

@jackwh any chance you can share the request ID for that change? I've not been able to reproduce this proration with a few different configurations.

@jackwh
Copy link
Contributor Author

jackwh commented Nov 16, 2020

@cjavilla-stripe I just replicated this with test customer cus_INR5pRT6BfmAoL. This was using the current version of Cashier (without any changes from this PR applied).

Cashier made two requests, the first was req_jZNJ07GJwd3p24, and the second was req_PiX4aLndE3CnTp. This triggered 3 events:

  • evt_1HoCGQLa4OrFV0Zvv9KsjHtF
  • evt_1HoCGQLa4OrFV0Zvc41j74gP
  • evt_1HoCGQLa4OrFV0ZvCEV608Uk

Let me know if there's anything else I can provide. Thanks!

@driesvints driesvints marked this pull request as draft November 20, 2020 14:09
@driesvints
Copy link
Member

Marked this as a draft until Stripe has gotten back to us.

@driesvints
Copy link
Member

@jackwh I finally have more info here from Stripe. It turns out that you're indeed very right and that proration does apply when migrating from tax_percentage to Tax Rates on subscription items. So this PR makes completely sense and you indeed need to apply noProrate when executing this method.

Now I have one final question I want to check off with you and Stripe before going ahead with this PR: shouldn't we just apply noProrate to this method call all the time? Under what situation would someone want it differently? If we apply noProrate behavior all the time we can just update the PR to not use the method call but set none as a fixed value.

@jackwh
Copy link
Contributor Author

jackwh commented Nov 24, 2020

@driesvints Thanks for the update! One example where you would want to prorate would be when a tax rate changes, whilst subscription charges are being accrued on a daily basis.

Here in the UK, on January 4th 2011 the rate of VAT changed from 17.5% to 20%. It doesn't change often, but should this happen again you would want the ability to prorate.

As you can't update the percentages of an existing Tax Rate object (only archive it), you would need to create a new Tax Rate (e.g. for VAT now at 20%), and assign that new rate to subscriptions by syncing it. Charges incurred up to the day the new tax came in to effect would still be charged at the old rate of 17.5%, and costs incurred from that day forward would be at the new rate. The next invoice would show these charges prorated correctly.

@driesvints
Copy link
Member

@jackwh FYI: I'm waiting for a reply from @cjavilla-stripe on this before we continue.

@driesvints driesvints marked this pull request as ready for review December 3, 2020 17:00
@driesvints
Copy link
Member

I've talked to @cjavilla-stripe and we both agree with your points. I've marked this PR as ready now. Thanks a lot for your patience with this!

We've decided to keep the default behavior as in your PR with create_prorations which is the default API behavior of Stripe and leave it up to the user to choose what they want to do.

@taylorotwell this one can be merged now. You can ignore the tests which are failing because this is a fork.

@taylorotwell taylorotwell merged commit 0166ec2 into laravel:12.x Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants